Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use gosu and execs for execution #101

Merged
merged 4 commits into from
Dec 4, 2021
Merged

Use gosu and execs for execution #101

merged 4 commits into from
Dec 4, 2021

Conversation

bewing
Copy link
Contributor

@bewing bewing commented Nov 29, 2021

Use exec at the end of scripts in order to simplify the process chain
and signal handling. With this change, a SIGTERM sent by docker to
PID 1 will be propagated to the server binary for a clean shutdown.

Resolves #99

@bewing bewing changed the title Use exec for run.sh and binary calls WIP: Use exec for run.sh and binary calls Nov 29, 2021
@bewing
Copy link
Contributor Author

bewing commented Nov 29, 2021

Want to do some more testing later (fresh data folder, user connect test, etc) before merging.

@bewing bewing marked this pull request as draft November 29, 2021 15:55
@wolveix
Copy link
Owner

wolveix commented Nov 29, 2021

Thanks for taking the time to open this PR! Let me know when you feel it's ready to be merged and has been tested sufficiently, and I'll test it my end too :)

@mitchellmaler
Copy link
Contributor

Good call on the exec usage. Will test this locally.

@bewing
Copy link
Contributor Author

bewing commented Nov 30, 2021

Testing has shown that docker stop is still resulting in the process immediately exiting instead of the server binary performing an orderly shutdown. The latest changeset makes a slight change to run.sh, so that if desired, a user can override the UID/GID via docker run and use run.sh as the entrypoint, resulting in the server as PID 1.

eg, running
docker run --rm --name satisfactory.service -e MAXPLAYERS=8 -e STEAMBETA=false -v /mnt/local/storage:/config --user=1000:1000 --entrypoint /home/steam/run.sh -p 7777:7777/udp -p 15000:15000/udp -p 15777:15777/udp wolveix/satisfactory-server:latest

results in a running container satisfactory.service that can be issued a stop command. Adding -d is recommended if not using systemd or another system to control containers.

@bewing
Copy link
Contributor Author

bewing commented Dec 1, 2021

Going to take one more crack at this tonight after doing some research on sudo, signals, process groups and setsid. If that doesn't work, may have to just accept the partial solution in changeset 5cb8740

@bewing bewing marked this pull request as ready for review December 2, 2021 15:20
@bewing
Copy link
Contributor Author

bewing commented Dec 2, 2021

I played with this for another couple hours, and was unfortunately unable to get signal passing to work with the sudo call. Merging this partial fix does allow admins to bypass the sudo call after init.sh has been run at least once to set permissions on the volume mount.

@bewing bewing changed the title WIP: Use exec for run.sh and binary calls Use exec for binary call Dec 2, 2021
@bewing bewing changed the title Use exec for binary call Use gosu and execs for execution Dec 2, 2021
@bewing
Copy link
Contributor Author

bewing commented Dec 2, 2021

More testing welcome, but it does appear to start the server correctly, and the environment appears intact:

$ docker exec -it testing /bin/bash
root@14bfcff225ce:/config# gosu steam env
PUID=1000
HOSTNAME=14bfcff225ce
SERVERGAMEPORT=7777
SERVERQUERYPORT=15777
PWD=/config
AUTOPAUSE=true
HOMEDIR=/home/steam
STEAMAPPID=1690800
SERVERIP=0.0.0.0
AUTOSAVEONDISCONNECT=true
AUTOSAVENUM=3
GAMECONFIGDIR=/config/gamefiles/FactoryGame/Saved
PGID=1000
STEAMBETA=false
CRASHREPORT=true
STEAMCMDDIR=/home/steam/steamcmd
SERVERBEACONPORT=15000
MAXPLAYERS=4
TERM=xterm
USER=steam
SKIPUPDATE=false
SHLVL=1
AUTOSAVEINTERVAL=300
DISABLESEASONALEVENTS=false
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
GAMESAVESDIR=/home/steam/.config/Epic/FactoryGame/Saved/SaveGames
DEBUG=false
_=/usr/sbin/gosu
HOME=/home/steam

@wolveix
Copy link
Owner

wolveix commented Dec 2, 2021

Thanks for your continued efforts! Couple of questions. Firstly, why did you duplicate the mkdir into run.sh? This serves no purpose as far as I can tell.

Secondly, with the inclusion of gosu, is signal trapping functioning correctly? If not, I want to avoid including unnecessary dependencies. Your last response isn't overly clear, so I just wanted to clarify :)

Edit: Reading over gosus source, it looks good and should be exactly what we need.

@bewing
Copy link
Contributor Author

bewing commented Dec 2, 2021

Thanks for your continued efforts! Couple of questions. Firstly, why did you duplicate the mkdir into run.sh? This serves no purpose as far as I can tell.

This leaves open the possibility of changing the entrypoint when running a container, and still being able to run /home/steam/run.sh if the end-user so desires. It does not appear to have a negative effect.

Secondly, with the inclusion of gosu, is signal trapping functioning correctly? If not, I want to avoid including unnecessary dependencies. Your last response isn't overly clear, so I just wanted to clarify :)

I tested by starting a container in the foreground, and issuing a stop from another terminal. The server received the signal and performed an orderly shutdown. I also tested on a detached container, and it was able to receive the signal as well.

[2021.12.02-19.18.53:466][  0FUnixPlatformMisc::RequestExitWithStatus
FUnixPlatformMisc::RequestExit
]LogWorld: BeginTearingDown for /Temp/Untitled_0
...
[2021.12.02-19.19.23:475][467]LogModuleManager: Shutting down and abandoning module RSA (3)
[2021.12.02-19.19.23:475][467]LogContentStreaming: Display: There are 1 unreleased StreamingManagers
[2021.12.02-19.19.23:478][467]LogExit: Exiting.
[2021.12.02-19.19.23:478][467]LogCore: FUnixPlatformMisc::RequestExit(bForce=false, ReturnCode=143)
Exiting abnormally (error code: 143)
Assertion failed: !HasCommands() [File:Runtime/RHI/Public/RHICommandList.h] [Line: 3816]

Pure virtual function called!
Signal 6 caught.

@wolveix
Copy link
Owner

wolveix commented Dec 2, 2021

Thanks for your continued efforts! Couple of questions. Firstly, why did you duplicate the mkdir into run.sh? This serves no purpose as far as I can tell.

This leaves open the possibility of changing the entrypoint when running a container, and still being able to run /home/steam/run.sh if the end-user so desires. It does not appear to have a negative effect.

It doesn't have a negative effect as such, it's just annoying to have a duplicate command as both instances must then be maintained.

If we're using gosu, then I think the possiblity of changing the entrypoint becomes an unnecessary and out-of-scope requirement to cater for.

@bewing
Copy link
Contributor Author

bewing commented Dec 2, 2021

It doesn't have a negative effect as such, it's just annoying to have a duplicate command as both instances must then be maintained.

If we're using gosu, then I think the possiblity of changing the entrypoint becomes an unnecessary and out-of-scope requirement to cater for.

Fair enough. I'll remove it from the changeset.
While I'm making changes, does it make sense to pass "$@" through the exec stack to allow additional server binary command line parameters to be passed via docker's CMD parameter?

ie docker run --rm ... wolveix/satisfactory-server:latest -foo=bar would result in Engine/Binaries/Linux/UE4Server-Linux-Shipping FactoryGame -log -NoSteamClient ... -foo=bar being eventually executed?

@wolveix
Copy link
Owner

wolveix commented Dec 2, 2021

Appreciate it!

No, you're probably right. Please remove it.

Ensure that if run.sh is called directly with an existing mounted
/config, we can correctly execute the server.  This allows us to
manually set the uid/gid of steam and switch the entrypoint, causing the
server binary to be UID 1.  This allows graceful stopping of the server
when `docker stop` is issued.  Testing has unfortunately shown multiple
issues with propagating signals through the sudo call present in init.sh
gosu (/~https://github.com/tianon/gosu) is optimized for container
environments to correct use exec and setsid to allow for signal handling
to occur correctly.

Pass any arguments to docker run command to the server binary.
Copy link
Contributor

@msladek msladek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this and works as intended for my setup, much appreciated!

run.sh Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Co-authored-by: Marc Sladek <marc.sladek@synventis.com>
@wolveix wolveix merged commit d9a1ac6 into wolveix:main Dec 4, 2021
@wolveix
Copy link
Owner

wolveix commented Dec 4, 2021

Thanks for your hard work @bewing!

@bewing bewing deleted the exec branch December 5, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Container not gracefully shutting down on SIGTERM and is being killed instead
4 participants